-
Notifications
You must be signed in to change notification settings - Fork 603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Permissionless batches recovery #1555
base: develop
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
can you add more description about this pr |
Will do so once it's in a better shape. This is still a draft PR and and such many things are still changing. Once ready, I'll provide a high-level description + how it relates to the changes made in this PR :) |
@@ -202,6 +204,7 @@ func (o *Chunk) InsertChunk(ctx context.Context, chunk *encoding.Chunk, codecVer | |||
parentChunkStateRoot = parentChunk.StateRoot | |||
} | |||
|
|||
fmt.Println("insertChunk", totalL1MessagePoppedBefore, chunkIndex, parentChunkHash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer using log.Info
, it's better shown in grafana.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sure that's just a debug print actually 🙃
// Create batch for the created chunks. We only allow 1 batch it needs to be submitted (and finalized) with a proof in a single step. | ||
log.Info("Creating batch for chunks", "from", latestFinalizedChunk.Index+1, "to", latestChunk.Index) | ||
|
||
batchProposer.TryProposeBatch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might this not be feasible? e.g. if the batch is super large, it may exceed the blob size limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that's a good point. I was thinking about how we could solve this. but probably it is more user friendly if we allow to create multiple batches but then handle the proof generation and the submission one by one. remember in permissionless mode a bundle is of size 1 as the proof needs to be delivered together with the batch submission.
func restoreMinimalPreviousState(cfg *config.Config, chunkProposer *watcher.ChunkProposer, batchProposer *watcher.BatchProposer) (*orm.Chunk, *orm.Batch, error) { | ||
log.Info("Restoring previous state with", "L1 block height", cfg.RecoveryConfig.L1BlockHeight, "latest finalized batch", cfg.RecoveryConfig.LatestFinalizedBatch) | ||
|
||
// TODO: make these parameters -> part of genesis config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, they are already in genesis.json
.
|
||
chunkProposer := watcher.NewChunkProposer(subCtx, cfg.L2Config.ChunkProposerConfig, genesis.Config, db, registry) | ||
batchProposer := watcher.NewBatchProposer(subCtx, cfg.L2Config.BatchProposerConfig, genesis.Config, db, registry) | ||
//bundleProposer := watcher.NewBundleProposer(subCtx, cfg.L2Config.BundleProposerConfig, genesis.Config, db, registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not needed (i.e. only finalizing batches are sufficient)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to provide the proof together with the submission of the batch. not sure if we also need to generate bundles (of size 1) to then kickstart the proving process?
log.Info("Latest finalized batch from L1 contract", "latest finalized batch", latestFinalizedBatch, "at latest finalized L1 block", latestFinalizedL1Block) | ||
|
||
// 4. Get batches one by one from stored in DB to latest finalized batch. | ||
receipt, err := l1Client.TransactionReceipt(context.Background(), common.HexToHash(latestDBBatch.CommitTxHash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to the pruning of txn index, not sure if we may need an archive node or fallback to fetch the whole block for this.
// 2. Make sure that the specified batch is indeed finalized on the L1 rollup contract and is the latest finalized batch. | ||
// TODO: enable check | ||
//latestFinalizedBatch, err := reader.LatestFinalizedBatch(latestFinalizedL1Block) | ||
//if cfg.RecoveryConfig.LatestFinalizedBatch != latestFinalizedBatch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious about why setting cfg.RecoveryConfig.LatestFinalizedBatch
instead of using reader.LatestFinalizedBatch
as the start point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, can probably remove it and make the configuration a bit easier. initially, I wanted the user to specify L1 block and the latest finalized batch so that the user knows where the (minimal) recovery process is starting from and there's no "magic" happening (e.g. if there's another batch committed in the meantime).
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tag
incommon/version.go
been updated or have you addedbump-version
label to this PR?Breaking change label
Does this PR have the
breaking-change
label?